Audio: ASRC: Code reorganization #8514

andrula-song · 2023-11-23T03:19:38Z

Move all ASRC related header files into src/audio/asrc folder.
split IPC version related code to asrc_ipc3.c and asrc_ipc4.c.

src/audio/asrc/asrc_ipc3.c

keqiaozhang · 2023-11-23T07:49:32Z

SOFCI TEST

RanderWang

LGTM

src/audio/CMakeLists.txt

src/audio/asrc/asrc.c

src/audio/asrc/asrc.h

src/audio/asrc/asrc.c

lgirdwood

We can make the Cmake/Kconfig changes later as there are other PRs that need to be merged 1st.

lgirdwood · 2023-11-24T13:16:12Z

src/audio/CMakeLists.txt

@@ -174,12 +174,14 @@ set(sof_audio_modules mixer volume src asrc eq-fir eq-iir dcblock crossover tdfb
 # sources for each module
 if(CONFIG_IPC_MAJOR_3)
 	set(volume_sources volume/volume.c volume/volume_generic.c volume/volume_ipc3.c)
+	set(asrc_sources asrc/asrc_ipc3.c)


@singalsu @andrula-song @btian1 These should really be in src/audio/module_name/CMakefile.txt (same rule applies to Kconfig too)

I agree, shall we use a separate pr to do the clean up once we finished all module converter.
this is because the in-complete of Cmakelist in module, some does not have, some does not used.

@lgirdwood , this change is based on search, so I request use a separate PR to track clean up src/audio/cMakeList.txt

Hmm, "asrc_sources" was not set before this PR, so why do it now. It seems we already go to the asrc subdirectory, so I don't see why we need to add individual source files here.

actually, it is first set in line 196 as "set(asrc_sources asrc/asrc.c asrc/asrc_farrow.c asrc/asrc_farrow_generic.c)". Adding here is just to keep the asrc_sources right after I refine ASRC code.

Ack @andrula-song . I still don't understand why we set the module sources list twice, but clearly this was not added in this PR, so should not block this one. Let's handle separately.

I still don't understand why we set the module sources list twice,

Same "LIBRARY" explanation as in #8595 (comment), isn't it?

andrula-song · 2023-11-27T05:32:45Z

SOFCI TEST

lgirdwood

LGTM, needs the cleanups in a different patch.

src/audio/asrc/asrc.c

lyakh

I like it much better with the typedef!

src/audio/asrc/asrc_ipc4.c

zephyr/CMakeLists.txt

RanderWang

Why we need to move it to ASRC folder ? Thanks!

keqiaozhang · 2023-11-30T08:49:46Z

SOFCI TEST

lgirdwood

I'm good here.

lgirdwood · 2023-11-30T17:12:46Z

src/audio/asrc/asrc.c

-#else
-	const struct ipc4_asrc_module_cfg *ipc_asrc =  mod_data->cfg.init_data;
-#endif
+	const ipc_asrc_cfg *ipc_asrc = mod_data->cfg.init_data;


This is a code change (I know, no impact), but should come in another patch.

lgirdwood · 2023-11-30T17:16:32Z

src/audio/asrc/asrc.h

+	asrc_proc_func asrc_func;		/* ASRC processing function */
+};
+
+#ifndef CONFIG_IPC_MAJOR_4


This is fine for now. We can incrementally update if needed.

lgirdwood · 2023-11-30T17:17:44Z

src/audio/asrc/asrc.h

+#ifdef CONFIG_IPC_MAJOR_4
+typedef struct ipc4_asrc_module_cfg ipc_asrc_cfg;
+#else
+typedef struct ipc_config_asrc ipc_asrc_cfg;


Fine for now, we can split this further later if needed.

zephyr/CMakeLists.txt

src/audio/asrc/asrc.h

Thank you

marc-hb · 2023-12-04T07:17:20Z

ALSA plugin build failure https://github.com/thesofproject/sof/actions/runs/7081524859/job/19272763291?pr=8514 discussed in #8538

btian1 · 2023-12-06T07:36:28Z

@lyakh , with typedef, there are codestyle errors, do you still insist use typedef, how about #define, or directly use # if IPC3?

lyakh · 2023-12-06T11:17:09Z

@lyakh , with typedef, there are codestyle errors, do you still insist use typedef, how about #define, or directly use # if IPC3?

@btian1 I never insisted on that, it was my proposal. I still think it's ok to ignore code style complaints in this case, but if others disagree we can also use a #define which would be my second preference.

plbossart · 2023-12-06T15:52:25Z

src/audio/asrc/asrc_config.h

@@ -16,7 +16,7 @@
 #if defined __XCC__
 /* For xt-xcc */
 #include <xtensa/config/core-isa.h>
-#if XCHAL_HAVE_HIFI3 == 1
+#if XCHAL_HAVE_HIFI3 == 1 || XCHAL_HAVE_HIFI4 == 1


this isn't really a code reorganization... It's really not good practice to add such configuration changes in the middle or a code move...

This should be in a separate patch IMHO.

add a separate commit at last.

marc-hb · 2023-12-06T16:24:26Z

but if others disagree we can also use a #define which would be my second preference.

Checkpatch is not always right: https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html

Checkpatch is not always right. Your judgement takes precedence over checkpatch messages. If your code looks better with the violations, then its probably best left alone.

I wish people obsessed at least as much about actual test results as about checkpatch. "Do not add any new typedef" looks especially wrong. Maybe it makes sense in the Linux kernel but not here. The less pre-processing the better; the indirection is a nightmare to debug (recent example: zephyrproject-rtos/zephyr#65953)

kv2019i

Mostly look good, a few minor comments inline.

kv2019i · 2023-12-07T11:58:42Z

src/audio/CMakeLists.txt

@@ -174,12 +174,14 @@ set(sof_audio_modules mixer volume src asrc eq-fir eq-iir dcblock crossover tdfb
 # sources for each module
 if(CONFIG_IPC_MAJOR_3)
 	set(volume_sources volume/volume.c volume/volume_generic.c volume/volume_ipc3.c)
+	set(asrc_sources asrc/asrc_ipc3.c)


Hmm, "asrc_sources" was not set before this PR, so why do it now. It seems we already go to the asrc subdirectory, so I don't see why we need to add individual source files here.

src/audio/asrc/CMakeLists.txt

kv2019i · 2023-12-07T12:00:20Z

src/audio/asrc/asrc_ipc3.c

+
+void asrc_update_buffer_format(struct comp_buffer *buf_c, struct comp_data *cd)
+{
+	//IPC3 don't need to update audio stream format here.


C-style comments would be better

kv2019i · 2023-12-07T12:00:26Z

src/audio/asrc/asrc_ipc3.c

+
+void asrc_set_stream_params(struct comp_data *cd, struct sof_ipc_stream_params *params)
+{
+	//IPC3 don't need to set audio stream params here.


Move the ASRC related header files into ASRC folder. Signed-off-by: Andrula Song <[email protected]>

Use typedef ipc_asrc_cfg to replace the splited IPC version asrc module config structure. Signed-off-by: Andrula Song <[email protected]>

Move IPC3 related code to asrc_ipc3.c and IPC4 related code to asrc_ipc4.c. Signed-off-by: Andrula Song <[email protected]>

Make HiFi4 share c source files with HiFi3. Signed-off-by: Andrula Song <[email protected]>

singalsu

Looks good, thanks! With assumption Kai's findings fixed.

kv2019i · 2023-12-11T10:54:27Z

src/audio/CMakeLists.txt

@@ -174,12 +174,14 @@ set(sof_audio_modules mixer volume src asrc eq-fir eq-iir dcblock crossover tdfb
 # sources for each module
 if(CONFIG_IPC_MAJOR_3)
 	set(volume_sources volume/volume.c volume/volume_generic.c volume/volume_ipc3.c)
+	set(asrc_sources asrc/asrc_ipc3.c)


Ack @andrula-song . I still don't understand why we set the module sources list twice, but clearly this was not added in this PR, so should not block this one. Let's handle separately.

btian1 reviewed Nov 23, 2023

View reviewed changes

src/audio/asrc/asrc_ipc3.c Show resolved Hide resolved

andrula-song force-pushed the asrc-reorg branch from 100cd0b to 17291ee Compare November 23, 2023 08:41

andrula-song marked this pull request as ready for review November 24, 2023 01:44

andrula-song requested review from marc-hb, RanderWang, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners November 24, 2023 01:44

RanderWang reviewed Nov 24, 2023

View reviewed changes

src/audio/CMakeLists.txt Outdated Show resolved Hide resolved

btian1 reviewed Nov 24, 2023

View reviewed changes

lyakh reviewed Nov 24, 2023

View reviewed changes

src/audio/asrc/asrc.c Show resolved Hide resolved

andrula-song force-pushed the asrc-reorg branch from 17291ee to 764500f Compare November 24, 2023 10:00

andrula-song changed the title ~~Audio: ASRC: Code clean~~ Audio: ASRC: Code reorganization Nov 24, 2023

lgirdwood reviewed Nov 24, 2023

View reviewed changes

lgirdwood reviewed Nov 27, 2023

View reviewed changes

src/audio/asrc/asrc.c Outdated Show resolved Hide resolved

andrula-song force-pushed the asrc-reorg branch from 764500f to 232dde8 Compare November 28, 2023 01:52

lyakh reviewed Nov 28, 2023

View reviewed changes

src/audio/asrc/asrc_ipc4.c Outdated Show resolved Hide resolved

andrula-song force-pushed the asrc-reorg branch from 232dde8 to 0e3f1f2 Compare November 29, 2023 01:45

marc-hb previously requested changes Nov 29, 2023

View reviewed changes

zephyr/CMakeLists.txt Outdated Show resolved Hide resolved

marc-hb mentioned this pull request Nov 30, 2023

Audio: multiband_drc: multiband drc code re-organize and split #8526

Merged

This comment was marked as outdated.

Sign in to view

RanderWang reviewed Nov 30, 2023

View reviewed changes

lgirdwood approved these changes Nov 30, 2023

View reviewed changes

plbossart requested changes Nov 30, 2023

View reviewed changes

src/audio/asrc/asrc.h Show resolved Hide resolved

andrula-song force-pushed the asrc-reorg branch from 0e3f1f2 to b91436d Compare December 1, 2023 03:30

andrula-song force-pushed the asrc-reorg branch from b91436d to 79b9546 Compare December 4, 2023 03:10

lyakh approved these changes Dec 6, 2023

View reviewed changes

plbossart reviewed Dec 6, 2023

View reviewed changes

andrula-song force-pushed the asrc-reorg branch from 79b9546 to b62e218 Compare December 7, 2023 09:32

kv2019i reviewed Dec 7, 2023

View reviewed changes

andrula-song added 4 commits December 8, 2023 09:54

Audio: ASRC: Move header files into ASRC folder

9774997

Move the ASRC related header files into ASRC folder. Signed-off-by: Andrula Song <[email protected]>

Audio: ASRC: Use typedef to replace IPC version asrc cfg

35fc3fb

Use typedef ipc_asrc_cfg to replace the splited IPC version asrc module config structure. Signed-off-by: Andrula Song <[email protected]>

Audio: ASRC: Split IPC version code to speclfic file

4c9e82f

Move IPC3 related code to asrc_ipc3.c and IPC4 related code to asrc_ipc4.c. Signed-off-by: Andrula Song <[email protected]>

Audio: ASRC: Make HiFi4 share c source files with HiFi3

756314f

Make HiFi4 share c source files with HiFi3. Signed-off-by: Andrula Song <[email protected]>

andrula-song force-pushed the asrc-reorg branch from b62e218 to 756314f Compare December 8, 2023 01:58

lgirdwood approved these changes Dec 8, 2023

View reviewed changes

plbossart approved these changes Dec 8, 2023

View reviewed changes

singalsu reviewed Dec 11, 2023

View reviewed changes

singalsu approved these changes Dec 11, 2023

View reviewed changes

kv2019i approved these changes Dec 11, 2023

View reviewed changes

kv2019i merged commit bfb6780 into thesofproject:main Dec 11, 2023
40 of 44 checks passed

kv2019i mentioned this pull request Dec 14, 2023

[FEATURE] organize audio module source code to per-module directories #8630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio: ASRC: Code reorganization #8514

Audio: ASRC: Code reorganization #8514

andrula-song commented Nov 23, 2023

keqiaozhang commented Nov 23, 2023

RanderWang left a comment

lgirdwood left a comment

lgirdwood Nov 24, 2023

btian1 Nov 27, 2023

btian1 Nov 29, 2023

kv2019i Dec 7, 2023

andrula-song Dec 8, 2023 •

edited

Loading

kv2019i Dec 11, 2023

marc-hb Dec 11, 2023

andrula-song commented Nov 27, 2023

lgirdwood left a comment

lyakh left a comment

This comment was marked as outdated.

RanderWang left a comment

keqiaozhang commented Nov 30, 2023

lgirdwood left a comment

lgirdwood Nov 30, 2023

lgirdwood Nov 30, 2023

lgirdwood Nov 30, 2023

marc-hb commented Dec 4, 2023

btian1 commented Dec 6, 2023

lyakh commented Dec 6, 2023

plbossart Dec 6, 2023

andrula-song Dec 7, 2023

marc-hb commented Dec 6, 2023 •

edited

Loading

kv2019i left a comment

kv2019i Dec 7, 2023

kv2019i Dec 7, 2023

kv2019i Dec 7, 2023

singalsu left a comment •

edited

Loading

kv2019i Dec 11, 2023

Audio: ASRC: Code reorganization #8514

Audio: ASRC: Code reorganization #8514

Conversation

andrula-song commented Nov 23, 2023

keqiaozhang commented Nov 23, 2023

RanderWang left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrula-song Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrula-song commented Nov 27, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

RanderWang left a comment

Choose a reason for hiding this comment

keqiaozhang commented Nov 30, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Dec 4, 2023

btian1 commented Dec 6, 2023

lyakh commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Dec 6, 2023 • edited Loading

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrula-song Dec 8, 2023 •

edited

Loading

marc-hb commented Dec 6, 2023 •

edited

Loading

singalsu left a comment •

edited

Loading